Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symfony Mailer Support #219

Merged
merged 1 commit into from Nov 27, 2019
Merged

Symfony Mailer Support #219

merged 1 commit into from Nov 27, 2019

Conversation

Baachi
Copy link
Contributor

@Baachi Baachi commented Nov 23, 2019

Hey Guys!

I created a symfony mailer reporter because swiftmailer is "official" dead I would think.
The SwiftMailer reporter is still available, so no BC breaks :)

#SymfonyHackday

@Baachi Baachi changed the title Add support for symfony 5 🚀 Symfony Mailer Support Nov 23, 2019
@Baachi
Copy link
Contributor Author

Baachi commented Nov 25, 2019

Okay it seems that I have to adjust the Travis file. The mailer component does not run on php versions lower than 7.3. :/

@kbond
Copy link
Collaborator

kbond commented Nov 26, 2019

Thanks @Baachi, the travis issue is not with php < 7.3 but that symfony/mailer requires symfony components 4.4+.

Copy link
Collaborator

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I'd like declare(strict_types=1); removed - I'd like to follow the Symfony code standards.

@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this?

@@ -0,0 +1,99 @@
<?php

declare(strict_types=1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove.

@@ -0,0 +1,50 @@
<?php

declare(strict_types=1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove.

@@ -0,0 +1,65 @@
<?php

declare(strict_types=1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove.

Copy link
Collaborator

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes to the travis config to better handle different symfony versions.

  1. Please rebase
  2. See https://github.com/doctrine/DoctrineBundle/blob/f6c5c7678b0ad3a6af8478e7903f3de3beeaf258/.travis.yml#L42 to see how we can add the mailer only on tests using symfony 4.3+ (I can do this step if you'd like)

try {
$mailerDefinition = $container->findDefinition('mailer');
} catch (ServiceNotFoundException $e) {
throw new MissingPackageException('To enable mail reporting you have to install the "swiftmailer/swiftmailer" or "symfony/mailer".');
Copy link
Collaborator

@kbond kbond Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just throw a \LogicException here? Don't know that we need a custom exception for this.

@@ -103,6 +112,48 @@ public function testMailer()
$this->assertContainerBuilderHasService('liip_monitor.reporter.swift_mailer');
}

public function testSymfonyMailer()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mark as skipped if MailerInterface does not exist.

composer.json Outdated
@@ -42,7 +42,8 @@
"symfony/asset": "^3.4|^4.0",
"symfony/templating": "^3.4|^4.0",
"phpunit/phpunit": "^7.0",
"symfony/finder": "^3.4|^4.0"
"symfony/finder": "^3.4|^4.0",
"symfony/mailer": "^4.3 || ^5.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, we'll add with travis if symfony > 4.3

@kbond
Copy link
Collaborator

kbond commented Nov 27, 2019

Thanks @Baachi, I made these changes.

@kbond kbond merged commit 91c7032 into liip:master Nov 27, 2019
}

try {
$mailerDefinition = $container->findDefinition('mailer');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Baachi , @kbond

I am unable to make the bundle work when I enable the mailer configuration.

I am using Symfony 4.4 (symfony/mailer 4.4, symfony/mailgun-mailer 4.4), LiipMonitorBundle 2.11.1

Regarding the Symfony documentation

In the load() method, all services and parameters related to this extension will be loaded. This method doesn't get the actual container instance, but a copy. This container only has the parameters from the actual container. After loading the services and parameters, the copy will be merged into the actual container, to ensure all services and parameters are also added to the actual container.

The container copy won't never have any mailer definition, so the exception is always raise.

Does it need a CompilerPass to load the mailer configuration instead?

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @ixarlie, it looks like we should just be setting the config parameters in the extension and like you suggest, have a compiler pass to create the service.

@Baachi, are you able to take a look?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kbond , @Baachi finally I've opened a new pull request addressing this issue. #227

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants